Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Add exports field to packages #43521

Closed
wants to merge 29 commits into from

Conversation

DiegoAndai
Copy link
Member

Follow up on #41596 (comment). Draft to create CI builds to test with.

CC: @Janpot

@DiegoAndai DiegoAndai added the core Infrastructure work going on behind the scenes label Aug 29, 2024
@DiegoAndai DiegoAndai self-assigned this Aug 29, 2024
@DiegoAndai DiegoAndai marked this pull request as draft August 29, 2024 20:29
@mui-bot
Copy link

mui-bot commented Aug 29, 2024

Netlify deploy preview

Bundle size report

Bundle size will be reported once CircleCI build #742065 finishes.

Generated by 🚫 dangerJS against 659a479

@DiegoAndai
Copy link
Member Author

@Janpot I'm testing #41596 (comment). Initially the issue doesn't seem to be fixed by removing the internal package.json files.

I'm doing the same test we did a few months back:

Interestingly, back then, esmExternals: false fixed the issue, but now it doesn't, so progress I guess 😂

I'll do a debugging session, but I'm letting you know in case you want to test it on your side in the meantime.

@Janpot
Copy link
Member

Janpot commented Aug 30, 2024

@DiegoAndai I'v been working on this over the past week in

I've extracted some of the issues that popped up as they can be released separately:

I followed your steps to reproduce with the following dependencies:

"@mui/icons-material": "https://pkg.csb.dev/mui/material-ui/commit/d4953d45/@mui/icons-material",
"@mui/material": "https://pkg.csb.dev/mui/material-ui/commit/d4953d45/@mui/material",
"@mui/material-nextjs": "https://pkg.csb.dev/mui/material-ui/commit/d4953d45/@mui/material-nextjs",

And both pnpm dev and pnpm build work, regardless of the value of esmExternals. Am I missing something? We should probably check what's the difference between both our PRs, e.g. I think I have a different method of dealing with next/document.

I'm still in the process of testing how X works but that looks promising so far. I'm also going to verify the vite issues Yep, that works now.

The one unknown I have left is whether I can remove esmExternals from our docs. Initial tests suggest no, but have to dig deeper into it.

@DiegoAndai
Copy link
Member Author

Great work @Janpot 🙌🏼

When I read #43264, I thought we needed this PR to test it, but let's keep #43264 as the ongoing initiative.

I tested with #43264 and it works, so I'm closing this PR.

@DiegoAndai DiegoAndai closed this Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants